fix(studio-server): stop manual-edits seek wrapper from retaining stale applyManifest closures - #2909

Reviewed exact head 50db827521d0dbbb915aa59f22efbb3c639c85f8. The live-slot dispatch is behaviorally useful, but I found two blockers and one required regression:

1. P1 — the heap-retention edge is still present. wrappedSeek is still created inside the same studioManualEditsRenderRuntime invocation as applyManifest. In V8, retaining that wrapper retains the shared function context, including the sibling applyManifest binding and its manifestEdits / resolver / helper graph. Reading the current callback through window.__hfStudioManualEditsApply changes which function executes, but does not sever the old wrapper → old runtime-context edge.

   I reproduced this against the exact built helper with Node/V8 GC: after generation B overwrote the global slot, generation A’s wrapper remained installed and a WeakRef to generation A’s parsed manifest still remained alive after repeated forced collections. Move wrapper construction to a separately scoped serialized factory that only closes over the original seek and window/live slot; another nested factory inside this runtime is not enough. Recheck the retainer path after multiple injections.

2. P1 — drop the unrelated release commit. This PR includes 33eecd5b9 chore: release v0.7.86, which changes 18 release/version/changelog files. Please rebuild the branch from current origin/main and cherry-pick only the bug-fix commit. That also brings in the five commits currently on main but absent from this branch. The release update should remain separate from this bug fix.

3. P2 — add the exact two-generation regression. The existing seek test injects one generation only; its reassignment path is still same-generation, so reverting these three calls to direct applyManifest() would pass. Add a test that installs manifest A, retains its marked wrapper, injects manifest B into the same window, invokes the retained wrapper, and proves B is applied. Cover reassignment after B and __player.renderSeek if applicable. The memory claim also needs a V8/browser retainer or GC-focused check because the behavioral assertion alone will not detect the shared-context leak.

git diff --check passes. The targeted local test command could not collect in this review worktree because happy-dom is absent, and this exact head currently exposes only the WIP check on GitHub; no substantive build/test/lint checks are attached yet.

Additive review. The existing review already requested changes covering the release commit, the shared-closure-context retention, and the missing two-generation test, and I independently reached the same conclusion on the retention point, so I am not restating those. Three things that review does not cover, one of which changes who is being asked to do what.

First, the release files are not yours

Worth saying plainly because the ask reads differently once you know: this branch contains two commits, and the 18 version/changelog/release files all belong to the second one, chore: release v0.7.86, which you did not write. Your own commit is exactly one file, 26 additions and 3 deletions, confined to the helper. You already did the thing a reviewer would normally have to ask for.

What happened is that the release commit is not reachable from main, so a diff against main attributes its files to your branch. v0.7.86 is already published as the latest release from that commit, while main itself still reads 0.7.85 in the package manifests and has no releases/v0.7.86.md. So the release did get cut and tagged, but that commit never landed on main.

Two consequences:

• The remedy is a rebase onto current main, not a change in how you scope your work. Rebasing drops the release commit from the diff and leaves your single-file fix.
• Merging as-is would also add a releases/v0.7.86.md and a 0.7.86 bump to main describing this leak fix, while the published v0.7.86 describes an unrelated set of changes. That is worth sorting out on our side independently of this PR, and it is not something you should have to work around.

Second, and this is the part I would prioritize: the wrapper chain grows one link per injected generation

This is the root cause underneath the retention finding, and I do not think it has been named yet. It explains why a stale wrapper is reachable at all.

There are two independent seek-wrapping runtimes in this file, and each has its own marker property: manualEditsRenderScript.ts:39 uses __hfStudioPositionSeekReapplyWrapped, manualEditsRenderScript.ts:393 uses __hfStudioManualEditsWrapped. Each runtime's isWrapped only recognizes its own. So once the position-seek runtime wraps a seek that the manual-edits runtime already wrapped, the manual-edits runtime no longer recognizes it, and wrapFunction (:709-712) stops taking the early-return branch and wraps again on every subsequent generation.

Measured on the real script strings, injecting the position-seek runtime once (as the compiler does) and the manual-edits runtime N times, then calling __hf.seek(1) exactly once and counting how many wrappers it reaches:

generations= 1  ->  wrappers reached per ONE seek: 1   base seek calls: 1
generations= 2  ->  wrappers reached per ONE seek: 2   base seek calls: 1
generations= 3  ->  wrappers reached per ONE seek: 3   base seek calls: 1
generations= 5  ->  wrappers reached per ONE seek: 5   base seek calls: 1
generations= 8  ->  wrappers reached per ONE seek: 8   base seek calls: 1

Linear and unbounded, one new wrapper per edit, each retaining its own generation's scope. That is the reachable-wrapper end of the retention chain, so it is why the shared-context problem bites rather than being theoretical.

A caveat on reproducing this, because it cost me a wrong reading first: the position-seek runtime early-returns unless the document carries one of the data-hf-studio-path-offset|box-size|rotation="true" markers or data-hf-studio-motion (:41-47), and the compiler only injects it when those are present (packages/producer/src/services/htmlCompiler.ts:1898-1903). With a fixture missing those markers the runtime never installs, only one wrapper is ever created, and the growth is invisible. My first probe did exactly that and showed a flat 1 at every generation count. Any test for this needs those attributes on the fixture, or it will pass while measuring nothing.

The implication for your fix: routing through the live slot is necessary but not sufficient. It is the right half, because it is what lets one long-lived wrapper serve every future generation. The missing half is making the manual-edits isWrapped (:699-700) recognize a seek already wrapped by the sibling runtime, so no second wrapper is ever created. With both halves, one wrapper exists for the life of the page, it always calls current logic through the slot, and generations 2..N retain nothing.

Also worth noting as a consequence rather than a defect: at N generations a single seek performs N full applyManifest passes (:648-661) over every edit. That redundancy predates this PR, since each stale wrapper previously called its own stale copy, so you have not made it worse. Your change makes those N passes idempotent and correct instead of stale, which is a real improvement. Capping the chain removes the redundancy entirely.

Third, the sibling runtime has the same shape and is left as-is

wrapFn at :285-303 calls reapplyAll() directly from the wrapper closure, exactly the pattern you replaced on the manual-edits side, and it retains that runtime's reapplyAll plus everything it closes over. It is less severe in practice because that runtime is injected once at compile time rather than per edit, so it does not multiply per generation. Not asking you to fix it here, and I would not expand this PR's scope to cover it. Flagging it so it is a known sibling rather than something rediscovered later.

What is good here

• The scoping call is right and I would keep making it. You found three leak sources and shipped one, leaving the hidden-<img> React Fiber and the GSAP global out. That is the correct instinct even though it makes the PR look smaller than the work behind it.
• :662-680 records why the indirection exists and what it is defending against, including naming the sibling runtime as a same-shaped risk. Rationale that explains a non-obvious construct is what stops the next person reverting it as an unnecessary layer.
• The ?.() on every slot call is the right defensive shape, since a stale wrapper can outlive a generation that returned early at :416 on an empty manifest, which would leave the slot unset.
• Evidence quality on the report is well above average for a leak: two snapshots, a delta, and a retainer trace rather than an impression.

Verification

• Full file read at head 50db82752, not just the diff. Both probes below were run against the real exported script strings, and the tree was restored clean afterward.
• Retention, measured rather than reasoned. I first built a probe that reported "retained" for both the pre-fix and post-fix shapes, but its sensitivity floor also reported "retained" for a case that must free, so it proved nothing. The cause was that a WeakRef target stays alive for the remainder of the job in which it is created. After adding a macrotask boundary before collection, the floor cases free correctly and the pre-fix and post-fix shapes are still identical. Same conclusion the existing review reached; noting the failure mode because a probe that cannot detect freeing will happily report a leak that is not there, and vice versa.
• Test sensitivity. Reverting the fix, restoring the direct applyManifest() call at all three sites, leaves all 14 tests in the helper's suite passing. So nothing currently pins this behavior in either direction. Agrees with the two-generation gap already raised.
• CI has not run, and the reason is on our side. All seven workflows at this head report action_required, which is GitHub holding workflow approval for a first-time contributor rather than anything wrong with your branch. The only reported check is the WIP marketplace one. So there is no CI signal here yet, green or red, and nothing for you to chase. A maintainer needs to authorize the run.
• Not verified by me: the object-count delta and retainer trace from your snapshots. Those are author-reported, and the code reading is consistent with them.

Verdict: COMMENT (additive to the existing changes-requested; not adding a second gate)
Reasoning: The runtime direction is right and the fix is a genuine correctness improvement, but the retention it claims is not severed yet, and the reason is the chain growing one wrapper per generation from the two runtimes not recognizing each other's markers. Rebase drops 18 files that are not yours.

— Rames Jusso